fix: return json error response body as object #238
Conversation
There was a problem hiding this comment.
Everything looks great, but I think we need to be tolerant of content-type values that might contain additional context stuff at the end.
We do this in a few other places (generator, cores, etc.) and we might want to also introduce this approach in the node core. We probably have several other situations (either in the node core or other repos) where we should be doing regex matches instead of strict "===", so it's probably not critical that we do this now in this PR. Just mentioning this since I noticed it while reviewing this PR.
lib/request-wrapper.ts
Outdated
| */ | ||
| function contentTypeIsJSON(headers: IncomingHttpHeaders): boolean { | ||
| const contentType = headers['content-type'] || ''; | ||
| return contentType.toLowerCase() === 'application/json'; |
There was a problem hiding this comment.
I think whenever we check Content-Type for specific values, rather than doing a strict "===", we should tolerate mimetypes like application/json; charset=utf8. In most situations (generator, cores, etc.) where we need to check Content-Type, we use a regex type comparison to account for the optional stuff at the end.
I did a quick check of the node core code, and I don't think we have anything like this place yet. Perhaps we could just introduce a utility function isJsonMimetype().
There was a problem hiding this comment.
Yes, great call. I thought about that after I made the changes but I forgot to go back and implement it. I'm glad you pointed that out!
| it('should convert string response bodies to objects if content is JSON', () => { | ||
| const newAxiosError = makeCopy(basicAxiosError); | ||
| newAxiosError.response.data = '{ "errorMessage": "some error" }'; | ||
| expect(newAxiosError.response.headers['content-type']).toBe('application/json'); |
There was a problem hiding this comment.
let's try a content-type that has extra stuff (e.e. application/json; charset=utf8.
dpopp07
force-pushed
the
dp/json-error-responses
branch
from
6b65ef5
to
3fc11aa
Compare
|
Okay @padamstx, your comments have been addressed! |
dpopp07
force-pushed
the
dp/json-error-responses
branch
from
3fc11aa
to
b335754
Compare
Error response bodies are currently returned as strings, even when the content is JSON. This is a bug - the SDK should return the result as an object. This commit adds logic to return the result as an object, converting it if necessary. It also updates the error object to use the "result" field for the response body, in order to be consistent with the success path. The "body" field is still set for backward-compatibility but is marked as deprecated. Additionally, for both success and error responses, if the content is JSON but the result cannot be parsed as an object, an exception will be raised, alerting the user that the service returned a malformatted body. Signed-off-by: Dustin Popp <dpopp07@gmail.com>
dpopp07
force-pushed
the
dp/json-error-responses
branch
from
b335754
to
dd50441
Compare
|
Latest changes look good (I already approved). |
## [4.0.4](v4.0.3...v4.0.4) (2023-02-27) ### Bug Fixes * return json error response body as object ([#238](#238)) ([6f201b7](6f201b7))
|
🎉 This PR is included in version 4.0.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Error response bodies are currently returned as strings, even when the content is JSON. This is a bug - the SDK should return the result as an object. This commit adds logic to return the result as an object, converting it if necessary.
It also updates the error object to use the "result" field for the response body, in order to be consistent with the success path. The "body" field is still set for backward-compatibility but is marked as deprecated.
Additionally, for both success and error responses, if the content is JSON but the result cannot be parsed as an object, an exception will be raised, alerting the user that the service returned a malformed body.
cc @ricellis